with_param #159
with_param #159
Conversation
serprex
force-pushed
the
with-param
branch
2 times, most recently
from
8891fb7
to
32382c9
Compare
serprex
force-pushed
the
with-param
branch
2 times, most recently
from
edbd788
to
abf96dd
Compare
|
Aside from the simpler types, the following needs testing:
I'd maybe also not mix it with |
serprex
force-pushed
the
with-param
branch
2 times, most recently
from
135598b
to
70aa220
Compare
|
So already found issue with basic tests. String Note once a string is contained it is expected to be quoted in params: ClickHouse/clickhouse-connect#159 |
serprex
force-pushed
the
with-param
branch
4 times, most recently
from
5466a0e
to
19ba376
Compare
src/lib.rs
Outdated
| @@ -160,6 +161,12 @@ impl Client { | |||
| self | |||
| } | |||
|
|
|||
| pub fn with_param(self, name: &str, value: impl Serialize) -> Result<Self, String> { | |||
There was a problem hiding this comment.
What's the purpose of Client::with_param? Parameters are specific for queries, not the whole client
There was a problem hiding this comment.
Generally, but if someone wants to share parameter between queries queries they can put it on the client
For example, in a multi tenant app, tenantid could be put as a parameter in the function creating client
There was a problem hiding this comment.
There was a problem hiding this comment.
What's the obscure outcomes you're thinking of? Better people use this than roll it themselves with options
This keeps things simple compared to https://github.com/citusdata/django-multitenant or https://github.com/citusdata/activerecord-multi-tenant tho those hook into ORM to inject predicates
|
We also need tests for And need to add some examples. |
src/query.rs
Outdated
| @@ -195,6 +195,12 @@ impl Query { | |||
| self.client.add_option(name, value); | |||
| self | |||
| } | |||
|
|
|||
| pub fn with_param(self, name: &str, value: impl Serialize) -> Result<Self, String> { | |||
There was a problem hiding this comment.
This method must be well documented. Also, we should specify the difference between param() and bind() (probably in a dedicated section on Query and provide links to that section in docs of both methods).
There was a problem hiding this comment.
added some docs, signature should somewhat communicate difference given ? is positional while server side parameters are key/value
serprex
force-pushed
the
with-param
branch
4 times, most recently
from
2eaaa70
to
759a7e0
Compare
|
@loyd, I'd like to merge this one. WDYT? |
Small wrapper around
with_optionforimpl Serialize + Bind